[SPARK-9750][MLlib] Improve equals on SparseMatrix and DenseMatrix by feynmanliang · Pull Request #8042 · apache/spark

feynmanliang · 2015-08-07T22:11:05Z

Adds unit test for equals on mllib.linalg.Matrix class and equals to both SparseMatrix and DenseMatrix. Supports equality testing between SparseMatrix and DenseMatrix.

@mengxr

feynmanliang · 2015-08-07T22:12:04Z

To start the discussion; should a SparseMatrix and a DenseMatrix be equal if they contain the same underlying content (i.e. do we define == as equality in semantics or as equality in representation)?

SparkQA · 2015-08-07T22:51:58Z

Test build #40202 has finished for PR 8042 at commit 78f9426.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

feynmanliang · 2015-08-09T20:11:05Z

Jenkins test please

feynmanliang · 2015-08-09T22:22:20Z

Jenkins test this please

jkbradley · 2015-08-10T18:57:32Z

To start the discussion; should a SparseMatrix and a DenseMatrix be equal if they contain the same underlying content (i.e. do we define == as equality in semantics or as equality in representation)?

If we follow Breeze's example, then we should define equality in semantics, not representation. I think this PR is valid.

SparkQA · 2015-08-10T19:31:03Z

Test build #1420 has finished for PR 8042 at commit 78f9426.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jkbradley · 2015-08-10T19:32:59Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala

This case should be for Matrix, not just SparseMatrix.

OK.

Will also change DenseMatrix.equals.

SparkQA · 2015-08-10T21:44:03Z

Test build #40320 has finished for PR 8042 at commit 22782df.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jkbradley · 2015-08-10T22:00:37Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala

I like what you had before better. Using Breeze, I think you'd avoid converting to dense representations. With toArray, it becomes dense.

Totally forgot about that, thanks.

jkbradley · 2015-08-11T00:18:36Z

mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala

You could replace toArray with toBreeze here too, in case "o" is sparse

jkbradley · 2015-08-11T18:29:53Z

LGTM pending tests

SparkQA · 2015-08-11T19:04:16Z

Test build #40488 has finished for PR 8042 at commit bb70d5e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jkbradley · 2015-08-11T19:49:08Z

Merging with master and branch-1.5

Adds unit test for `equals` on `mllib.linalg.Matrix` class and `equals` to both `SparseMatrix` and `DenseMatrix`. Supports equality testing between `SparseMatrix` and `DenseMatrix`. mengxr Author: Feynman Liang <fliang@databricks.com> Closes #8042 from feynmanliang/SPARK-9750 and squashes the following commits: bb70d5e [Feynman Liang] Breeze compare for dense matrices as well, in case other is sparse ab6f3c8 [Feynman Liang] Sparse matrix compare for equals 22782df [Feynman Liang] Add equality based on matrix semantics, not representation 78f9426 [Feynman Liang] Add casts 43d28fa [Feynman Liang] Fix failing test 6416fa0 [Feynman Liang] Add failing sparse matrix equals tests (cherry picked from commit 520ad44) Signed-off-by: Joseph K. Bradley <joseph@databricks.com>

mengxr · 2015-08-12T04:44:30Z

@feynmanliang The contract for Java's Object is that a.equals(b) implies a.hashCode == b.hashCode. So usually we need to implement both. The problem with hashCode is that we shouldn't compute it based on all values, which could be very expensive. You can use the implementation of Vector.hashCode as a template, but that requires some changes to avoid hash code collisions.

feynmanliang · 2015-08-12T22:41:34Z

@mengxr made SPARK-9919 to track this

Adds unit test for `equals` on `mllib.linalg.Matrix` class and `equals` to both `SparseMatrix` and `DenseMatrix`. Supports equality testing between `SparseMatrix` and `DenseMatrix`. mengxr Author: Feynman Liang <fliang@databricks.com> Closes apache#8042 from feynmanliang/SPARK-9750 and squashes the following commits: bb70d5e [Feynman Liang] Breeze compare for dense matrices as well, in case other is sparse ab6f3c8 [Feynman Liang] Sparse matrix compare for equals 22782df [Feynman Liang] Add equality based on matrix semantics, not representation 78f9426 [Feynman Liang] Add casts 43d28fa [Feynman Liang] Fix failing test 6416fa0 [Feynman Liang] Add failing sparse matrix equals tests

Feynman Liang added 3 commits August 7, 2015 15:05

Add failing sparse matrix equals tests

6416fa0

Fix failing test

43d28fa

Add casts

78f9426

jkbradley reviewed Aug 10, 2015
View reviewed changes

feynmanliang changed the title ~~[SPARK-9750][MLlib] Add equals to SparseMatrix~~ [SPARK-9750][MLlib] Improve equals on SparseMatrix and DenseMatrix Aug 10, 2015

Add equality based on matrix semantics, not representation

22782df

jkbradley reviewed Aug 10, 2015
View reviewed changes

Sparse matrix compare for equals

ab6f3c8

jkbradley reviewed Aug 11, 2015
View reviewed changes

Breeze compare for dense matrices as well, in case other is sparse

bb70d5e

asfgit closed this in 520ad44 Aug 11, 2015

feynmanliang deleted the SPARK-9750 branch August 17, 2015 19:10

Conversation

feynmanliang commented Aug 7, 2015

Uh oh!

feynmanliang commented Aug 7, 2015

Uh oh!

SparkQA commented Aug 7, 2015

Uh oh!

feynmanliang commented Aug 9, 2015

Uh oh!

feynmanliang commented Aug 9, 2015

Uh oh!

jkbradley commented Aug 10, 2015

Uh oh!

SparkQA commented Aug 10, 2015

Uh oh!

jkbradley Aug 10, 2015

Choose a reason for hiding this comment

Uh oh!

feynmanliang Aug 10, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 10, 2015

Uh oh!

jkbradley Aug 10, 2015

Choose a reason for hiding this comment

Uh oh!

feynmanliang Aug 11, 2015

Choose a reason for hiding this comment

Uh oh!

jkbradley Aug 11, 2015

Choose a reason for hiding this comment

Uh oh!

feynmanliang Aug 11, 2015

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Aug 11, 2015

Uh oh!

SparkQA commented Aug 11, 2015

Uh oh!

jkbradley commented Aug 11, 2015

Uh oh!

mengxr commented Aug 12, 2015

Uh oh!

feynmanliang commented Aug 12, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants